Work on adding show options to code list pop up - #1379

Configuration used: CodeRabbit UI
Review profile: CHILL

js/formidable_admin.js (1)

Line range hint 1-3070: Good use of module pattern, but consider breaking it down

The frmAdminBuildJS function implements the module pattern, which is a good practice for encapsulation and organizing code. It creates a closure that contains private functions and variables, while returning an object with public methods and properties. This approach helps in maintaining a clean global namespace and controlling access to internal functionality.

However, the function is quite large and contains many nested functions and variables. Consider breaking this down into smaller, more focused modules or sub-modules. This could improve readability, maintainability, and make it easier to test individual components.

Here's a high-level suggestion for refactoring:

1. Identify logical groupings of functionality (e.g., form building, settings, export/import).
2. Create separate modules for each group.
3. Use ES6 modules or a similar system to manage dependencies between these modules.
4. Maintain a main module that initializes and coordinates these sub-modules.

This approach would make the code more modular and easier to maintain in the long run.

Configuration used: CodeRabbit UI
Review profile: CHILL

Configuration used: CodeRabbit UI
Review profile: CHILL

js/formidable_admin.js (1)

2967-2987: 🛠️ Refactor suggestion

Add error handling for JSON parse

The getExcludeArray function parses JSON without error handling.

Add try/catch block around JSON.parse:

- const exclude = JSON.parse(box.getElementsByClassName('frm_code_list')[0].getAttribute('data-exclude'));
+ let exclude = [];
+ try {
+   const excludeAttr = box.getElementsByClassName('frm_code_list')[0].getAttribute('data-exclude');
+   exclude = excludeAttr ? JSON.parse(excludeAttr) : [];
+ } catch (e) {
+   console.error('Error parsing exclude data:', e);
+ }

js/formidable_admin.js (1)

2929-2931: 🛠️ Refactor suggestion

Fix potential undefined return in shouldShowFieldLabelShortcode

The optional chaining operator could return undefined instead of a boolean.

Add an explicit boolean conversion:

- return [ 'radio', 'checkbox', 'dropdown' ].includes( fieldType ) && document.getElementById( `separate_value_${fieldId}` )?.checked;
+ return [ 'radio', 'checkbox', 'dropdown' ].includes( fieldType ) && !! document.getElementById( `separate_value_${fieldId}` )?.checked;

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

js/src/admin/admin.js (4)

3050-3065: Feature flag truthiness can misfire; make it explicitly boolean

dataset.supportsShowShortcodes is a string and any non-empty value (including "0") is truthy. Prefer a presence or explicit boolean parse to avoid accidental enabling.

-const supportsShowShortcodes = document.getElementById( 'new_fields' ).dataset.supportsShowShortcodes;
+const newFieldsEl = document.getElementById( 'new_fields' );
+const supportsShowShortcodes = newFieldsEl?.hasAttribute( 'data-supports-show-shortcodes' ) &&
+  (newFieldsEl.dataset.supportsShowShortcodes === '' ||
+   /^(1|true|yes)$/i.test( newFieldsEl.dataset.supportsShowShortcodes ));

---

3113-3145: Minor UI polish: add spacing and accessible label to shortcode items

Currently the label text is concatenated directly after “[code]” with no space, and the link lacks an accessible label. Add a space node and aria-label to improve readability/AT support.

 function addCalcFieldLiToList( list, fieldId, code, label, fieldType ) {
   const anchor = a( {
     className: 'frm_insert_code',
     children: [
       span( {
         text: '[' + code + ']'
       } ),
-      document.createTextNode( label )
+      document.createTextNode( ' ' ),
+      document.createTextNode( label )
     ],
     data: {
       code
-    }
+    },
+    ariaLabel: label + ' shortcode ' + '[' + code + ']'
   } );

---

3068-3097: Scope OK; consider reflecting actual Name subparts to avoid dangling options

Nice addition. If practical, hide parts not enabled in the Name field’s current layout (e.g., no Middle) to reduce confusion in the popup.

Please confirm if the Name layout setting is available in DOM (eg, a per-field dropdown). If so, I can wire a small check to only add present parts.

---

3068-3097: Doc nit: replace “@SInCE x.x” with the release version

Update the JSDoc @since tags to the actual version used in this release.

Also applies to: 3099-3112, 3113-3145

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

js/src/admin/admin.js (1)

3096-3109: Dropdowns won’t show “label” shortcode — include 'select' in type check

The field type array omits 'select' (used for Dropdowns elsewhere), so [fieldId show=label] never appears for Dropdown fields.

Apply:

-function shouldShowFieldLabelShortcode( fieldType, fieldId ) {
-  return [ 'radio', 'checkbox', 'dropdown' ].includes( fieldType ) && !! document.getElementById( `separate_value_${ fieldId }` )?.checked;
-}
+function shouldShowFieldLabelShortcode( fieldType, fieldId ) {
+  // Support actual Dropdown type ('select'); keep 'dropdown' for safety if referenced elsewhere.
+  return [ 'radio', 'checkbox', 'select', 'dropdown' ].includes( fieldType ) &&
+    !! document.getElementById( `separate_value_${ fieldId }` )?.checked;
+}

Please also confirm this UI‑side check aligns with Pro behavior (comment noted that Pro needs no changes). [Based on past review comments]

js/src/admin/admin.js (3)

3110-3141: Minor UI nit: add spacing between “[code]” and label

Currently renders as “[123]My Field”. Add a leading space to the label text node.

-        document.createTextNode( label )
+        document.createTextNode( ' ' + label )

---

3064-3095: Name parts: consider hiding “Middle” when not enabled on the field

You always add first/middle/last. If the Name field is configured without a middle part, showing “(Middle)” may confuse. Optional: gate on the field’s current layout/settings (if available) before adding that shortcode. I can draft a DOM check if you confirm where the layout is stored.

---

3064-3141: Replace @SInCE x.x placeholders

Docblocks use “@SInCE x.x”. Replace with the actual next version tag used in this repo to keep docs accurate.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

js/src/admin/admin.js (2)

3056-3074: Localize the “(Label)” suffix in label shortcode entry and avoid hard‑coded English

The UI label for the [fieldId show=label] entry is built as fields[ i ].fieldName + ' (Label)', which is not translatable. This should use the existing i18n helpers so the suffix is localized and consistent with the rest of the admin UI.

A minimal change inline with existing patterns:

- if ( shouldShowFieldLabelShortcode( fields[ i ].fieldType, fields[ i ].fieldId ) ) {
-   addCalcFieldLiToList( list, fieldId, fields[ i ].fieldId + ' show=label', fields[ i ].fieldName + ' (Label)', fields[ i ].fieldType );
- }
+ if ( shouldShowFieldLabelShortcode( fields[ i ].fieldType, fields[ i ].fieldId ) ) {
+   addCalcFieldLiToList(
+     list,
+     fieldId,
+     fields[ i ].fieldId + ' show=label',
+     sprintf( __( '%s (Label)', 'formidable' ), fields[ i ].fieldName ),
+     fields[ i ].fieldType
+   );
+ }

This keeps the UX the same while making the suffix properly translatable.

---

3119-3121: Dropdowns won’t ever show [fieldId show=label] — type check is missing 'select'

shouldShowFieldLabelShortcode only allows ['radio', 'checkbox', 'dropdown']. In this codebase Dropdown fields actually use the type string 'select' elsewhere, so real Dropdown fields will never satisfy this check and their show=label entries won’t appear in the popup.

Recommend broadening the type list so Dropdowns are correctly supported:

-function shouldShowFieldLabelShortcode( fieldType, fieldId ) {
-  return [ 'radio', 'checkbox', 'dropdown' ].includes( fieldType ) && !! document.getElementById( `separate_value_${ fieldId }` )?.checked;
-}
+function shouldShowFieldLabelShortcode( fieldType, fieldId ) {
+  // Support actual Dropdown type ('select'); keep 'dropdown' in case it exists elsewhere.
+  return [ 'radio', 'checkbox', 'select', 'dropdown' ].includes( fieldType ) &&
+    !! document.getElementById( `separate_value_${ fieldId }` )?.checked;
+}

This aligns the new feature with existing field-type conventions and ensures show=label is available for Dropdowns when “Use separate values” is enabled.

js/src/admin/admin.js (3)

3077-3107: Name-part shortcodes helper looks good; consider aligning with actual name layout (optional)

maybeAddNamePartShortcodes cleanly adds [nameFieldId show=first|middle|last] entries and correctly uses __() for the sublabels, which matches the PR intent.

If you want to tighten UX later, you could read the Name field’s configured layout (eg, which parts are enabled) and skip generating entries for disabled parts so you don’t offer shortcodes like show=middle when no middle-name subfield will ever render. That would avoid confusing, non-functional options in the popup.

---

3132-3155: Helper for calc-field list items is solid; consider minor accessibility polish

addCalcFieldLiToList centralizes the markup for calc-field entries and correctly stores the raw shortcode in a data-code attribute, which makes the new hook API easier to use and keeps the DOM structure consistent.

Two small optional tweaks you might consider:

• Add an href="#" (or role="button" and tabindex="0") to the <a> so it’s keyboard-focusable and announced as interactive even if JS fails.
• If you expect add-ons to call this with non-string fieldId, you might document/normalize it to a string to avoid surprising class names.

No blockers here; helper is otherwise well-factored.

---

3062-3074: Replace @since x.x placeholders before release

The new frm_add_calc_field_shortcodes hook and helper docblocks still use @since x.x. Before tagging a release, these should be updated to the actual version number so downstream developers have accurate version metadata in the public JS API.

Purely documentation/i18n hygiene; no functional impact.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro